Enable additional code checks #29858
Conversation
|
The code-checks are being triggered in jenkins. |
|
This was spurred by the review of the L1 track trigger code, where some of these checks should really be automated. I assume some checks will be more controversial than others, and/or run into false positives in our code base, but let's start having a look ... |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29858/15415
|
|
A new Pull Request was created by @fwyzard (Andrea Bocci) for master. It involves the following packages: .clang-tidy @makortel, @smuzaffar, @cmsbuild, @Dr15Jones can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@smuzaffar is there a way to make the bot run the code checks using the modified .clang-tidy file ? |
fwyzard
force-pushed
the
enable_additional_code_checks
branch
from
0183239
to
c2dcc12
Compare
|
The code-checks are being triggered in jenkins. |
performance-faster-string-find: Optimize calls to std::string::find() and friends when the needle passed is a single character string literal. performance-for-range-copy: Finds C++11 for ranges where the loop variable is copied in each iteration but it would suffice to obtain it by const reference. performance-implicit-conversion-in-loop: This warning appears in a range-based loop with a loop variable of const ref type where the type of the variable does not match the one returned by the iterator. This means that an implicit conversion happens, which can for example result in expensive deep copies. performance-inefficient-algorithm: Warns on inefficient use of STL algorithms on associative containers. performance-inefficient-vector-operation: Finds possible inefficient std::vector operations (e.g. push_back, emplace_back) that may cause unnecessary memory reallocations. performance-move-const-arg: Warn about inefficient use of std::move, and suggest a fix that removes it. performance-unnecessary-copy-initialization: Finds local variable declarations that are initialized using the copy constructor of a non-trivially-copyable type, where it would suffice to obtain a const reference. performance-unnecessary-value-param: Flags value parameter declarations of expensive to copy types that are copied for each invocation, where it would suffice to pass them by const reference. modernize-make-unique: Finds the creation of std::unique_ptr objects explicitly calling a new expression, and replaces it with a call to std::make_unique. modernize-loop-convert: This check converts for(...; ...; ...) loops to use the new range-based loops in C++11. The MinConfidence option was already set to "reasonable". modernize-use-auto: Use the auto type specifier for variable declarations to improve code readability and maintainability. The MinTypeNameLength option is set to 16 characters. modernize-use-emplace: The check flags insertions to an STL-style container done by calling the push_back method with an explicitly-constructed temporary of the container element type. In this case, the corresponding emplace_back method results in less verbose and potentially more efficient code.
fwyzard
force-pushed
the
enable_additional_code_checks
branch
from
c2dcc12
to
5a6dc7f
Compare
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29858/15417
|
|
Pull request #29858 was updated. @makortel, @smuzaffar, @cmsbuild, @Dr15Jones can you please check and sign again. |
as in the past, it may be more effective to make a PR with code changes for some selected "representative" package or subsystem so that others could comment. |
|
Indeed - |
|
#29863 shows all the code changes from these additions, generated with scram b -j`nproc` llvm-ccdb
cat $CMSSW_BASE/compile_commands.json | grep '"file"' | cut -d'"' -f4 | xargs $(scram tool tag llvm LLVM_BASE)/share/clang/run-clang-tidy.py -header-filter "$CMSSW_BASE/src/.*" -j`nproc` -fix -formatI can generate smaller change sets (e.g. for a single package or subsystem, or only some of the options) if needed. |
|
Thanks @fwyzard, by quick look on random files in #29863 the new options look pretty good to me. For deployment I would nevertheless consider to split the set of options into two: straightforward ones, and the ones where the output is more subjective. To me We could anyway discuss this in the next core software meeting. |
I agree... also, the PR as is now is pretty huge, I might split the commits by option and/or by package. |
Right, at least for deployment it needs to be split up (perhaps with the aid of @cmsbuild? at least for the "straightforward options"). |
|
For posterity: in a future update, it would also be nice to include https://reviews.llvm.org/D54943 (const correctness, still in development) once it's finished, approved, and validated/stable. |
performance-faster-string-find:Optimize calls to std::string::find() and friends when the needle passed is a single character string literal.
performance-for-range-copy:Finds C++11 for ranges where the loop variable is copied in each iteration but it would suffice to obtain it by const reference.
performance-implicit-conversion-in-loop:This warning appears in a range-based loop with a loop variable of const ref type where the type of the variable does not match the one returned by the iterator. This means that an implicit conversion happens, which can for example result in expensive deep copies.
performance-inefficient-algorithm:Warns on inefficient use of STL algorithms on associative containers.
performance-inefficient-vector-operation:Finds possible inefficient std::vector operations (e.g. push_back, emplace_back) that may cause unnecessary memory reallocations.
performance-move-const-arg:Warn about inefficient use of std::move, and suggest a fix that removes it.
performance-unnecessary-copy-initialization:Finds local variable declarations that are initialized using the copy constructor of a non-trivially-copyable type, where it would suffice to obtain a const reference.
performance-unnecessary-value-param:Flags value parameter declarations of expensive to copy types that are copied for each invocation, where it would suffice to pass them by const reference.
modernize-make-unique:Finds the creation of std::unique_ptr objects explicitly calling a new expression, and replaces it with a call to std::make_unique.
modernize-loop-convert:This check converts for(...; ...; ...) loops to use the new range-based loops in C++11.
The
MinConfidenceoption was already set to "reasonable".modernize-use-auto:Use the auto type specifier for variable declarations to improve code readability and maintainability.
The
MinTypeNameLengthoption is set to 16 characters.modernize-use-emplace:The check flags insertions to an STL-style container done by calling the push_back method with an explicitly-constructed temporary of the container element type. In this case, the corresponding emplace_back method results in less verbose and potentially more efficient code.